JS_GetClass function #1215
JS_GetClass function #1215
Conversation
|
Exposing structs in the public API effectively freezes them; we can no longer add, remove, rename or reorder fields without breaking backwards compatibility. We're not at a stage yet where we actually promise API stability but eventually we will be. A better approach is to add something like
Therefore |
|
I'm happy to do whatever you tell me to. I had considered the atom approach as well. There was a JSClass typedef in quickjs.h that was never actually used, so I thought perhaps it was meant to be there but forgotten or some such. To me, the downsides to the int JS_GetClassName(JSRuntime *rt, JSClassId id, char *buf, int len) approach are an extra memcpy and an extra failure case for the caller to defend against (being told the provided length is insufficient). I personally like JSAtom JS_GetClassName(JSRuntime *rt, JSClassID id) because this allows the caller to choose whether to convert it to a cstring or to a JSValue string without having to pay for an extra copy from one to the other. Another option is const char *JS_GetClassName(JSContext *ctx, JSClassID id) with a note that the caller must JS_FreeCString it, but if the caller intended to convert it to JSValue then they pay for an unnecessary copy to cstring first. |
|
@saghul your API design thoughts, if you please? |
|
I like the |
|
Agreed. I think the approach return JSAtom is much nicer. |
|
Okay, @caturria make sure to |
|
Chiming in late... we do have other APIs what return names like the module name and script name for workers, and they return JSAtom, so there is that! |
|
Okay. Done. |
quickjs.c
Outdated
| JSAtom JS_GetClassName(JSRuntime *rt, JSClassID id) | ||
| { | ||
| if(!JS_IsRegisteredClass(rt, id)) { | ||
| return JS_ATOM_NULL; | ||
| } | ||
| JSAtom ret = rt->class_array[id].class_name; | ||
| JS_DupAtomRT(rt, ret); | ||
| return ret; | ||
| } |
There was a problem hiding this comment.
| JSAtom JS_GetClassName(JSRuntime *rt, JSClassID id) | |
| { | |
| if(!JS_IsRegisteredClass(rt, id)) { | |
| return JS_ATOM_NULL; | |
| } | |
| JSAtom ret = rt->class_array[id].class_name; | |
| JS_DupAtomRT(rt, ret); | |
| return ret; | |
| } | |
| JSAtom JS_GetClassName(JSRuntime *rt, JSClassID class_id) | |
| { | |
| if (JS_IsRegisteredClass(rt, class_id)) { | |
| return JS_DupAtomRT(rt, rt->class_array[class_id].class_name); | |
| } else { | |
| return JS_ATOM_NULL; | |
| } | |
| } |
There was a problem hiding this comment.
Github wouldn't let me directly commit the suggestions for whatever reason. I think I got 'em all?
Once again thanks for your patience with this. As a blind person especially whitespace-related inconsistencies are very difficult to even notice without proofreading letter-by-letter and even then they're easy to miss.
There was a problem hiding this comment.
There are still some left but I'll fix them manually when I merge your PR.
quickjs.h
Outdated
| JS_EXTERN int JS_NewClass(JSRuntime *rt, JSClassID class_id, const JSClassDef *class_def); | ||
| JS_EXTERN bool JS_IsRegisteredClass(JSRuntime *rt, JSClassID class_id); | ||
| /* Returns the class name or JS_ATOM_NULL if `id` is not a registered class. Must be freed with JS_FreeAtom. */ | ||
| JS_EXTERN JSAtom JS_GetClassName(JSRuntime *rt, JSClassID id); |
There was a problem hiding this comment.
| JS_EXTERN JSAtom JS_GetClassName(JSRuntime *rt, JSClassID id); | |
| JS_EXTERN JSAtom JS_GetClassName(JSRuntime *rt, JSClassID class_id); |
| } else { | ||
| return JS_ATOM_NULL; | ||
| } | ||
| } |
|
Merged with whitespace fixes in 307a59f, thanks! |
5 participants
Hi,
Thanks again for this fantastic library!
I would like to add a JS_GetClass function so that the host application can inspect the details of native classes. Useful especially for generation of context-specific error messages that include class names.
Are there any concerns regarding the definition of JSClass being moved to the public header?
I would also like to disclose that I am totally blind. Though I tried to match the style as best I could, your feedback on any stylistic inconsistencies I may have missed would be greatly appreciated.
Thank you.